-
Notifications
You must be signed in to change notification settings - Fork 739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sdk user story #5952
Sdk user story #5952
Conversation
Unit Test Results124 files ± 0 124 suites ±0 2m 1s ⏱️ -1s Results for commit 8f06415. ± Comparison against base commit 109b381. This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
322937a
to
d09acce
Compare
get() = keysBackupVersion?.version | ||
|
||
fun isEnabled(): Boolean | ||
fun isStucked(): Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it should be isStuck()
instead. The version with "ed" seems to not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When renaming a method which is exposed as an API, should we add an entry in the changelog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, will add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matrix-sdk-android/docs/packages.md
Outdated
@@ -1,3 +1,7 @@ | |||
# Package org.matrix.android.sdk._userstories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "_" should be removed since you renamed it without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks. Sadly Dokka does not complain :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* ### Required APIs: | ||
* - [org.matrix.android.sdk.api.Matrix] constructor | ||
*/ | ||
object Us000InitMatrix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have 2 questions:
- do the user stories really need to be
object
? I think simpleclass
could work and avoid loading of non used US at runtime. - Am I missing something or the US are not implemented yet? If not implemented, would't be better to make it
internal
or at least mention it in the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Yes this could be class
, that's better. Also maybe with a private constructor. Those classes are just here to be visible in the documentation.
This is not ideal, if you have a better idea...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just added a comment about whether we should add changelog entry for the renaming of the isStuck
method.
901d339
to
eb57680
Compare
Dokka will generate a better documentation (`Functions` and `Properties` are 2 distinct tab), and for Service it's better to have only `fun`
… and `state` has been renamed to `getState` and is now a fun.
Dokka does not complain about unknown package in this file :/
eb57680
to
53c83ab
Compare
/** | ||
* Either a session or an object containing data about registration stages. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have a basic documentation, you could replace by a single line comment. This avoids overloading the file with comment lines
/** | |
* Either a session or an object containing data about registration stages. | |
*/ | |
/** Either a session or an object containing data about registration stages. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely more compact. I will think about it for next changes.
With the current format, it's easier to add new lines.
Matrix SDKIntegration Tests Results:
|
This PR add some cleanup on the SDK documentation and on the API. See commit messages for the details.
Also it introduced the SDK user Story, with their documentation:
and for Us100SignIn for instance:
This is still WIP, but we can merge as it is and update later, to first check that the format is OK.